Avoid Substring() call in ConstructFunction #11962
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors ConstructFunction and related helpers in Expander.cs to eliminate intermediate Substring allocations by leveraging ReadOnlySpan<char> and ReadOnlyMemory<char>.
- Changed
ScanForClosingParenthesisandScanForClosingQuoteto operate on spans. - Updated
ExtractFunctionArgumentsto accept aReadOnlyMemory<char>and use span-based slicing. - Replaced most
Substringcalls with span/memory slicing, except for a remainingSubstringinConstructIndexerFunction.
Comments suppressed due to low confidence (3)
src/Build/Evaluation/Expander.cs:708
- The XML doc comment still refers to
string expression, but the parameter is nowReadOnlySpan<char>. Update the summary and<param>tags to match the new type.
private static int ScanForClosingParenthesis(ReadOnlySpan<char> expression, int index, out bool potentialPropertyFunction, out bool potentialRegistryFunction)
src/Build/Evaluation/Expander.cs:3925
- [nitpick] Mixing
stringand span/memory APIs can be confusing. Consider changing this method to acceptReadOnlyMemory<char>orReadOnlySpan<char>forexpressionFunction, so slicing logic is uniform and allocations are minimized.
private static void ConstructIndexerFunction(string expressionFunction, IElementLocation elementLocation, object propertyValue, int methodStartIndex, int indexerEndIndex, ref FunctionBuilder<T> functionBuilder)
src/Build/Evaluation/Expander.cs:828
- The new span-based parsing logic introduces multiple edge cases (nested properties, quotes, commas). Add unit tests covering mismatched parentheses/quotes and empty or complex argument lists to ensure correctness.
private static string[] ExtractFunctionArguments(IElementLocation elementLocation, string expressionFunction, ReadOnlyMemory<char> argumentsMemory)
rainersigwald
approved these changes
Jun 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #
Helps with
#10961
Context
The existing implementation of
ConstructFunction()calledSubstring()and passed that value to a handful of helper functions. It turns out that we can wrap the string inReadOnlyMemory<char>and pass that to the helper functions instead to avoid the additional allocation. Some of the functions needed to be updated to accept the new type.Before:
After:
Changes Made
Testing
Notes